Conversation
There was a problem hiding this comment.
Pull request overview
This PR migrates the atomic-insight-generated-answer component from Stencil to Lit as part of the ongoing framework migration. The component uses Coveo ML models to automatically generate answers to user queries in the Insight Panel context.
Changes:
- Migrated component implementation from Stencil (
.tsx) to Lit (.ts) - Added comprehensive unit tests and E2E tests for the migrated component
- Updated Storybook documentation with MDX file and new stories
- Added necessary test fixtures and utilities for the Insight generated answer controller
Reviewed changes
Copilot reviewed 15 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
atomic-insight-generated-answer.tsx |
Removed legacy Stencil implementation |
atomic-insight-generated-answer.ts |
New Lit-based implementation with same functionality |
atomic-insight-generated-answer.spec.ts |
Comprehensive unit tests covering all component features |
atomic-insight-generated-answer.new.stories.tsx |
Storybook stories for visual testing |
atomic-insight-generated-answer.mdx |
Documentation with usage examples |
e2e/atomic-insight-generated-answer.e2e.ts |
E2E tests for citation behavior and popover interactions |
e2e/page-object.ts |
Page object model for E2E testing |
e2e/fixture.ts |
Test fixture setup |
generated-answer-controller.ts |
Test fixture for mocking the headless controller |
mock.ts |
Added Insight endpoint for answer generation |
atomic-insight-generated-answer.tw.css.ts |
Tailwind styles (imports from common) |
atomic-insight-generated-answer.pcss |
Removed legacy PostCSS styles |
index.ts, lazy-index.ts |
Added exports for the new component |
custom-element-tags.ts |
Registered component tag |
...c/components/insight/atomic-insight-generated-answer/atomic-insight-generated-answer.spec.ts
Outdated
Show resolved
Hide resolved
...ic/src/components/insight/atomic-insight-generated-answer/atomic-insight-generated-answer.ts
Show resolved
Hide resolved
...c/components/insight/atomic-insight-generated-answer/atomic-insight-generated-answer.spec.ts
Outdated
Show resolved
Hide resolved
…d-answer/atomic-insight-generated-answer.spec.ts Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…d-answer/atomic-insight-generated-answer.spec.ts Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| @errorGuard() | ||
| render() { | ||
| const contentClasses = | ||
| 'mx-auto mt-0 mb-4 border border-neutral shadow-lg p-6 bg-background rounded-lg p-6 text-on-background'; |
There was a problem hiding this comment.
I think we might have done some changes/fix in the CSS. Double check the current version on main (cc. @SimonMilord ?)
There was a problem hiding this comment.
We're duplicating p-6 in the string.
Also, in the "search" version of the component we have:
'mx-auto mt-0 mb-4 border border-neutral shadow-lg bg-background rounded-lg text-on-background'
So, no p-6 at all.
I think renderAnswerContent should already handle internal padding with px-6 and py-3 (in the header) and px-6 and pb-6 (in the content container), so the p-6 we're adding to the outer
| </div> | ||
| `; | ||
| } | ||
| return html`${nothing}`; |
There was a problem hiding this comment.
| return html`${nothing}`; | |
| return nothing; |
| @errorGuard() | ||
| render() { | ||
| const contentClasses = | ||
| 'mx-auto mt-0 mb-4 border border-neutral shadow-lg p-6 bg-background rounded-lg p-6 text-on-background'; |
There was a problem hiding this comment.
We're duplicating p-6 in the string.
Also, in the "search" version of the component we have:
'mx-auto mt-0 mb-4 border border-neutral shadow-lg bg-background rounded-lg text-on-background'
So, no p-6 at all.
I think renderAnswerContent should already handle internal padding with px-6 and py-3 (in the header) and px-6 and pb-6 (in the content container), so the p-6 we're adding to the outer
| }); | ||
| } | ||
|
|
||
| private renderContent() { |
There was a problem hiding this comment.
The private method is kind of useless :D We could call renderAnswerContent directly.
| attribute: 'disable-citation-anchoring', | ||
| converter: booleanConverter, | ||
| }) | ||
| disableCitationAnchoring = false; |
There was a problem hiding this comment.
We're testing the disableCitationAnchoring default value + behavior in the spec file for the "search" version of the component and we're not in the "insight" version.
|
|
||
| if (this.hasNoAnswerGenerated) { | ||
| if ( | ||
| this.generatedAnswerState?.cannotAnswer && |
There was a problem hiding this comment.
This rendering path (where connotAnswer and hasCustomNoAnswerMessage are true) is not covered in the unit tests
It's not covered in the "search" version's tests either, so if you decide to add tests for this you should probably go back in atomic-generated-answer.spec.ts as well for good measure.
| expect(feedbackButtons.length).toBe(0); | ||
| }); | ||
|
|
||
| it('should render feedback buttons when not streaming and answer exists', async () => { |
There was a problem hiding this comment.
This test is semantically identical to "should render the feedback buttons when not streaming". It doesn't really add any value, so we could remove it.
|
|
||
| // TODO V4: KIT-5197 - Remove this test | ||
| it('should log validation warning when maxCollapsedHeight is updated to invalid value', async () => { | ||
| const consoleWarnSpy = vi |
There was a problem hiding this comment.
We can use the mockConsole util instead.
| }); | ||
|
|
||
| it('should warn when maxCollapsedHeight is out of range', async () => { | ||
| vi.spyOn(console, 'warn').mockImplementation(() => {}); |
There was a problem hiding this comment.
we're not asserting that 'warn' was called, only spying on it 😁
|
|
||
| test('should render citation when available', async ({generatedAnswer}) => { | ||
| const citationCount = await generatedAnswer.getCitationCount(); | ||
| expect(citationCount).toBeGreaterThan(0); |
There was a problem hiding this comment.
We could check for the exact expected count instead of greater than 0... but that's a minor issue.
We do this in a few places in the spec file.
| searchStatusState: {hasError: false}, | ||
| }); | ||
|
|
||
| if (retryButton) { |
There was a problem hiding this comment.
The test will silently pass if retryButton is falsy.
We should check with an assertion that it's in the document.
✅ Checklist
.mdxfileindex.tsandlazy-index.tsfiles.https://coveord.atlassian.net/browse/KIT-5267